Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use vel.xy().length() instead of norm(vel.x,vel.y) #18647

Merged
merged 15 commits into from
Sep 14, 2021

Conversation

hendjoshsr71
Copy link
Member

@hendjoshsr71 hendjoshsr71 commented Sep 12, 2021

This should be a non functional change.

I noticed the computation of norm(vel.x,vel.y) costs about 34--60 bytes each time it is used more than the equivalent vel.xy().length()

Multiply that by a bunch of times and you get back 1.2KB for Copter & 1KB for Plane and slightly less code.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find!

@@ -674,10 +674,10 @@ void Mode::land_run_horizontal_control()
// there is any position estimate drift after touchdown. We
// limit attitude to 7 degrees below this limit and linearly
// interpolate for 1m above that
float attitude_limit_cd = linear_interpolate(700, copter.aparm.angle_max, get_alt_above_ground_cm(),
const float attitude_limit_cd = linear_interpolate(700, copter.aparm.angle_max, get_alt_above_ground_cm(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated change. But a good one anyway.

@@ -1427,7 +1427,7 @@ void NavEKF3_core::alignMagStateDeclination()

// rotate the NE values so that the declination matches the published value
Vector3F initMagNED = stateStruct.earth_magfield;
ftype magLengthNE = norm(initMagNED.x,initMagNED.y);
ftype magLengthNE = initMagNED.xy().length();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there's no difference between norm and length() even for ftype?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will plead a little ignorance here. I was surprised by the savings.
For this specific case initMagNED it is an fytpe so there is no difference.

From what I can see in debugger and via inspection, T length(void) calls ftype norm(void) each function of so there is output type narrowing that is possible if the inputs are floats and ftype -> is double?

@rmackay9
Copy link
Contributor

I've reviewed the changes and it looks ok to me. It would be very laborious to test each line that is changed so perhaps we just rely on inspection.

I've added one question above about making sure that length and norm work fine for ftype.

@hendjoshsr71
Copy link
Member Author

I've reviewed the changes and it looks ok to me. It would be very laborious to test each line that is changed so perhaps we just rely on inspection.

I've added one question above about making sure that length and norm work fine for ftype.

This was 100% inspection and autotest only as it touches so many very specific test cases.

@tridge tridge merged commit 7a3425f into ArduPilot:master Sep 14, 2021
@hendjoshsr71 hendjoshsr71 deleted the pr/norm_to_xy_length branch September 14, 2021 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants